-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bulk get cached projection #7968
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #7968 +/- ##
============================================
- Coverage 28.26% 28.25% -0.01%
Complexity 2075 2075
============================================
Files 1276 1276
Lines 156178 156197 +19
Branches 3084 3084
============================================
- Hits 44137 44133 -4
- Misses 110200 110223 +23
Partials 1841 1841
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
I found a solution. We can use a transaction or pipe to execute all the commands simultaneously. Example with the CLI:
|
03edc77
to
7de773c
Compare
A better solution is to set up an |
7de773c
to
e681c02
Compare
d14e715
to
f22880b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed redis part, lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the nice improvements 👍
Description
The idea is to bulk-get the cached projection to limit the number of queries performed to Redis.
Benchmark
Condition:
The
path_projection
performance doesn't seem to change (1.2s).Queries performances:
I think in production, because of queries overhead the difference will be far more important.
The number of Redis queries is reduced by the number of valid train schedule. (2211 -> 1532 queries).
Next, I will do the same to retrieve the path and simulation from the cache (but this implies code refactoring).
Important
There is a limitation with this. I didn't find how to set the expiration time of Redis entry with a bulk command. So now the Redis entry expires after 1 week even if they're used recently.